pm: policy: latency: Add support for immediate action #81856
Conversation
nordic-krch
force-pushed
the
mram_latency
branch
from
cd1e954
to
e7dd4c6
Compare
| @@ -275,63 +267,140 @@ static inline int32_t pm_policy_next_event_ticks(void) | |||
| * The system will not enter any power state that would make the system to | |||
| * exceed the given latency value. | |||
| * | |||
| * If immediate control manager is provided request may trigger an action asynchronous action | |||
There was a problem hiding this comment.
What's the "immediate control manager"? I didn't find the term in Zephyr docs.
| */ | ||
| void pm_policy_latency_request_add(struct pm_policy_latency_request *req, | ||
| uint32_t value_us); | ||
| int pm_policy_latency_request_add_sync(struct pm_policy_latency_request *req, uint32_t value_us); |
There was a problem hiding this comment.
I would say the default function should be synchronous (without a suffix) and the special one, callable from IRQs, should be _async().
| * @retval 0 if request is applied. | ||
| * @retval 1 if request required immediate action that is not completed. Configured completion | ||
| * notification will inform about completion. |
There was a problem hiding this comment.
In async functions, I would expect that the callback is called regardless of the returned value. So this could be simplified to retval 0 if the request is pending. When the callback is applied, the callback notification is called with the result.
| */ | ||
| int pm_policy_latency_request_remove(struct pm_policy_latency_request *req); | ||
|
|
||
| /** @brief Immediate action manager for single threshold. |
There was a problem hiding this comment.
The "immediate" word used around max latency changes subscription is confusing to me. Is there any "delayed max latency changes subscription" to differentiate the "immediate" one from?
Maybe it would be enough to name this structure pm_policy_latency_change_binary_subscription?
| struct onoff_client cli; | ||
| void *internal; | ||
| uint32_t flags; |
There was a problem hiding this comment.
onoff_client and two 32-bit variables per latency requestor seem like high memory overhead. Wouldn't it be enough to track these data per subscriber instead of per requestor?
| int32_t thr; | ||
| }; | ||
|
|
||
| struct pm_policy_latency_immediate_ctrl { |
There was a problem hiding this comment.
And this one pm_policy_latency_change_subscription?
|
|
||
| /** | ||
| * @brief Subscribe to maximum latency changes. | ||
| * | ||
| * @param req Subscription request. | ||
| * @param cb Callback function (NULL to disable). | ||
| */ | ||
| void pm_policy_latency_changed_subscribe(struct pm_policy_latency_subscription *req, | ||
| pm_policy_latency_changed_cb_t cb); | ||
| int pm_policy_latency_immediate_ctrl_add(struct pm_policy_latency_immediate_ctrl *mgr); |
There was a problem hiding this comment.
And restore the name of this function? The previous one was much clearer to me.
Remove API for subscription to the latency change. This API was added to support executing immediate action on latency change but implementation was calling subscriber callbacks with interrupts locked which is not desirable. In general, it is non-trivial task to manage multiple requests and perform asynchronous action for the top request. On-off manager implements that for binary case. Additionally, API was missing a notifiction to the requestor when immediate action is completed and it is required for the used to know when requested latency requirement is fulfilled. Proposed API is adding a possible notification for request and update and adds option to add a manager for controlling immediate action. Added notifications make sense only if immediate action is used. Immediate action implements support for binary case where there is a single action that is performed when latency is below a given threshold. In future it can be extended to support gradual manager. Signed-off-by: Krzysztof Chruściński <[email protected]>
Add test for the case where latency change triggers an immediate action. Configuration is using a single threshold for triggering of the action. Signed-off-by: Krzysztof Chruściński <[email protected]>
|
For context, there is this PR #80580 which updates the behavior of the latency to be "sticky", meaning if the event time is set to a value less than current time, the latency request is immediately applied. Removing the callback I'm all for, since I have not found a usecase for it either :) Lastly, I'm working on a "competing"/parallel solution to this API which should cover the features added in this PR as well, it may make sense to wait for it to be ready for review to compare them (Draft PR within this week) :) |
Remove API for subscription to the latency change. This API was added to support executing immediate action on latency change but implementation was calling subscriber callbacks with interrupts locked which is not desirable. In general, it is non-trivial task to manage multiple requests and perform asynchronous action for the top request. For example, on-off manager implements that for binary case.
Additionally, API was missing a notification to the requester when immediate action is completed and it is required for the user to know when requested latency requirement is fulfilled.
Proposed API is adding a possible notification for request and update and adds option to add a manager for controlling immediate action. Added notifications make sense only if immediate action is used. Immediate action implements support for binary case where there is a single action that is performed when latency is below a given threshold. In future it can be extended to support gradual manager.
Latency policy API was added #42040 #48476 but never used beyond test and sample. One of the use cases is MRAM latency management (#81381) and current API could not handle that case for reasons described above.